-
Notifications
You must be signed in to change notification settings - Fork 646
[Scala3][WIP] Scala3 testing I #5083
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
fa56dfd to
2e3d722
Compare
seldridge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM. Some nits and there are some issues with CI.
| import org.scalatest.funspec.AnyFunSpec | ||
| import org.scalatest.matchers.must.Matchers | ||
| import org.scalatest.matchers.should.Matchers.convertToAnyShouldWrapper | ||
| // import org.scalatest.matchers.should.Matchers.convertToStringShouldWrapperForVerb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: please don't commit commented out code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this is a tricky one. the scalatest API is subtly different between Scala 2 and 3 leading to an either-or situation as to which ShouldWrapper to use. will revisit and fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check that ScalaTest doesn't already have a way to deal with this--it would be pretty bush league for such a popular library to screw this up. If they dont then we can polyfill it ourselves by adding a type alias for one or the other in the version-specific sources (e.g. in src/main/scala-2 define org.scalatest.matchers.should.Matchers.convertToStringShouldWrapperForVerb as a type alias for org.scalatest.matchers.should.Matchers.convertToAnyShouldWrapper).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs resolution
| val in = IO(Input(UInt(8.W))) | ||
| val fd = SimLog.file("logfile.log") | ||
| fd.printf("in = %d\n", in) | ||
| // fd.printf("in = %d\n", in) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this shouldn't be commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this file has so many lines added. Can you clarify what is going on?
a482a98 to
85ad8fa
Compare
7326a10 to
1920295
Compare
1920295 to
f12ce6d
Compare
| implicit val info: SourceLine = SourceLine("Foo", 3, 10) | ||
| 3.U >> -1 | ||
| } | ||
|
|
||
| class ErrorCaughtByFirtool extends RawModule { | ||
| implicit val info = SourceLine("Foo", 3, 10) | ||
| implicit val info: SourceLine = SourceLine("Foo", 3, 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be the more generic type SourceInfo, not the specific type.
| import org.scalatest.funspec.AnyFunSpec | ||
| import org.scalatest.matchers.must.Matchers | ||
| import org.scalatest.matchers.should.Matchers.convertToAnyShouldWrapper | ||
| // import org.scalatest.matchers.should.Matchers.convertToStringShouldWrapperForVerb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs resolution
| lazy val elements = SeqMap("" -> gen) | ||
| def underlying = elements.head._2 | ||
| def underlying = elements.head._2.asInstanceOf[T] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better for both of these would be:
val underlying = gen
lazy val elements = SeqMap("" -> underlying)It's not that this is fundamentally wrong, but people do look at the tests to get inspiration or see best practice sometimes and this cast is not good practice.
| @@ -3,7 +3,6 @@ | |||
| package chiselTests.naming | |||
|
|
|||
| import chisel3._ | |||
| import chisel3.aop.Select | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these aren't used, I'm all for deleting (and we should eventually enable scalafix to do it for us), but why did this need to be deleted to run the test with Scala 3?
| dut.io.numIn := VecInit(numsToAdd.map(x => x.asUInt(bitWidth.W))) | ||
| val sumCorrect = dut.io.numOut === (numsToAdd.reduce(_ + _) % (1 << bitWidth)).asUInt(bitWidth.W) | ||
| assert(sumCorrect) | ||
| chisel3.assert(sumCorrect) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary?
| final val io = IO { | ||
| new Bundle { | ||
| val a = Output(probe.Probe(Bool(), layers.Verification)) | ||
| val a = Output(probe.Probe(Bool(), chisel3.layers.Verification)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by why these are needed
|
|
||
| @nowarn("cat=deprecation") | ||
| val bb = Module(new BlackBox { | ||
| val bb = Module[BlackBox { def io: MyBundle }](new BlackBox { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be necessary due to Selectable right? Or did we decide not to apply that to modules?
| extends Module { | ||
| val inst = ModuleChoice(default)(alternateImpls) | ||
| val io = IO(inst.cloneType) | ||
| val inst: T = ModuleChoice[T](default, alternateImpls) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong, the API in src/main/scala-3/chisel3/choice/ModuleChoiceIntf.scala is incorrect and should be changed to match that in src/main/scala-2
Move spec files from src/test/scala-2 to src/test/scala/ and update for cross-compilation with Scala 3.
Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.6.x,5.x, or6.xdepending on impact, API modification or big change:7.0)?Enable auto-merge (squash)and clean up the commit message.Create a merge commit.